-
Notifications
You must be signed in to change notification settings - Fork 1
OUT-3072: support task association while creating task #1120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- [x] migration that renames column: viewers to associations and add shared column - [x] association selector and share with client toggle components - [x] task can be shared only if assocaition is selected and assignee is IU - [x] task can be created with no assignee and only associations - [x] zod validation to meet association and shared conditions
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| let associations: Associations = AssociationsSchema.parse(prevTask.associations) | ||
|
|
||
| const viewersResetCondition = shouldUpdateUserIds ? !!clientId || !!companyId : !prevTask.internalUserId | ||
| if (data.viewers) { | ||
| if (data.associations) { | ||
| // only update of viewers attribute is available. No viewers in payload attribute means the data remains as it is in DB. | ||
| if (viewersResetCondition || !data.viewers?.length) { | ||
| viewers = [] // reset viewers to [] if task is not reassigned to IU. | ||
| } else if (data.viewers?.length) { | ||
| viewers = await this.validateViewers(data.viewers) | ||
| if (viewersResetCondition || !data.associations?.length) { | ||
| associations = [] // reset viewers to [] if task is not reassigned to IU. | ||
| } else if (data.associations?.length) { | ||
| associations = await this.validateViewers(data.associations) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we simplify this? Maybe have a function that returns associates for us?
| protected async validateViewers(viewers: Viewers) { | ||
| if (!viewers?.length) return [] | ||
| const viewer = viewers[0] | ||
| protected async validateViewers(associations: Associations) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are just validating association. Maybe we should change the definition of the function. Could just accept the association and validate against it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you meant about the function name, this is still to-do. I didnt change the name as it might create impact on other places that uses this function. Will eventually implement this in future PRs.
| text: { | ||
| text: '#212B36', | ||
| textSecondary: '#6B6F76', | ||
| textSecondary: 'var(--text-secondary)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended? Doesn't this make the theme settings inconsistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is intended. The same value was repeated so made it reusable. I think this practice makes more sense. wdyt?
| checked: boolean | ||
| } | ||
|
|
||
| export const CustomToggle = ({ label, onChange, checked }: CustomToggleProps) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should name this component as CopilotToggle IMO. We have been doing this for other components like selector, tootltips. Just to avoid confusion in the future.
… smaller functions
Changes
Testing Criteria
Loom
Notes
Most of the files changes are basically because of name replace, from viewers to associations. Refer to the this commit for feature related changes